Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #37566 - Add UEFI Secure Boot Firmware to Libvirt #10321

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Fixes #37566 - Add UEFI Secure Boot Firmware to Libvirt #10321

merged 1 commit into from
Nov 25, 2024
+194 −2

Conversation

nofaralfasi
Copy link
Contributor

Requires:

This PR includes two commits:

  1. Add firmware selection option for Libvirt VM creation.
  2. Introduce a new firmware type for Secure Boot support.

When creating a new host in Foreman, after selecting Libvirt as the compute resource, a new option to select the VM's firmware will appear under the Virtual Machine tab. See the screenshot below for a demonstration:

image

Notes:

  1. For machines created through Foreman, enrolled-keys are enabled by default when Secure Boot is activated.
  2. For existing VMs, Secure Boot status is determined by the loader secure='yes' setting.

For more details: community post.

@nofaralfasi nofaralfasi requested a review from a team as a code owner September 15, 2024 10:46
This was referenced Sep 15, 2024
Closed
Merged
@nofaralfasi
Copy link
Contributor Author

Failing tests should be fixed automatically after fog/fog-libvirt#155 is merged.

This was referenced Sep 15, 2024
Closed
Open
@nofaralfasi
Copy link
Contributor Author

As noted in my comment on the VMware-related PR #10324 (comment), the same issue occurs with Libvirt. The Automatic firmware selection is not functioning correctly on the compute_attributes form.

@stejskalleos stejskalleos self-assigned this Sep 23, 2024
stejskalleos
stejskalleos requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments + it looks like failing tests are related.

Otherwise I was able to provision the Fedora 39 machine with BIOS, UEFI, and UEFI + Secure Boot.

app/models/compute_resources/foreman/model/libvirt.rb Show resolved Hide resolved
app/views/compute_resources_vms/form/libvirt/_base.html.erb Outdated Show resolved Hide resolved
@stejskalleos stejskalleos mentioned this pull request Sep 24, 2024
Merged
stejskalleos
stejskalleos previously approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍏 LGTM

The fog-libvirt part has to be merged and released first

@stejskalleos
Copy link
Contributor

Oh I missed the failing tests:

ActionView::Template::Error: Error making a connection to libvirt URI qemu://stam/system:
Call to virConnectOpen failed: Cannot read CA certificate '/etc/pki/CA/cacert.pem': No such file or directory

This needs to be fixed.

@nofaralfasi
Copy link
Contributor Author

Oh I missed the failing tests:

ActionView::Template::Error: Error making a connection to libvirt URI qemu://stam/system:
Call to virConnectOpen failed: Cannot read CA certificate '/etc/pki/CA/cacert.pem': No such file or directory

This needs to be fixed.

As I mentioned here, this should be fixed automatically after fog/fog-libvirt#155 is merged.

@stejskalleos
Copy link
Contributor

For me, the libvirt connection errors are also happening in my PR (#10351), where I don't touch Libvirt at all.

@nofaralfasi
Copy link
Contributor Author

The connection errors are unrelated to the changes in this PR.
@stejskalleos, can we proceed with merging this and address these errors in a follow-up PR?

@stejskalleos
Copy link
Contributor

@stejskalleos, can we proceed with merging this and address these errors in a follow-up PR?

Normally, I would proceed with a merge, but in this case, I would prefer to fix tests first and then merge PRs.
How can we be sure we haven't caused another issue that stays hidden behind current errors?

@nofaralfasi
Copy link
Contributor Author

Normally, I would proceed with a merge, but in this case, I would prefer to fix tests first and then merge PRs. How can we be sure we haven't caused another issue that stays hidden behind current errors?

The tests are running without any errors in my local environment, but I can fix them if you'd prefer.

stejskalleos
stejskalleos previously approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are running without any errors in my local environment, but I can fix them if you'd prefer.

Nah, let's get this in, it's been tested by multiple people, I think it's safe.

@stejskalleos
Copy link
Contributor

@nofaralfasi the Redmine issue check is failing, can you please squash the comments? I can't overpass that check.

@dosas
Copy link
Contributor

dosas commented Nov 18, 2024

The failing tests in this PR Call to virConnectOpen failed: Cannot read CA certificate '/etc/pki/CA/cacert.pem': No such file or directory resulting from the update to fog-libvirt 13 are now also present on the main branch

@stejskalleos
Copy link
Contributor

Libvirt test failures are fixed here: #10377

@stejskalleos
Copy link
Contributor

I approved PR, resolved all comments and yet still can't merge the issue :(
@ekohl can you push the button?

@ekohl
Copy link
Member

ekohl commented Nov 22, 2024

@stejskalleos there's a packaging change, but we already merged that. If you rebase then I'd expect it to be possible.

@nofaralfasi
Fixes #37566 - Add UEFI Secure Boot Firmware to Libvirt
81c51ee 
 - Add Firmware option to Libvirt VM creation.
 - Added a new firmware type for Secure Boot.
 - Enable `enrolled-keys` by default when Secure Boot is activated.
 - Added firmware-related methods to the ComputeResource model
   for shared use between VMware and Libvirt.
ekohl
ekohl approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test it myself, but the code reads well.

@nofaralfasi nofaralfasi merged commit 1feb571 into theforeman:develop Nov 25, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamruzicka adamruzicka adamruzicka left review comments

@stejskalleos stejskalleos stejskalleos approved these changes

@Gauravtalreja1 Gauravtalreja1 Gauravtalreja1 approved these changes

@ekohl ekohl ekohl approved these changes

Assignees

@stejskalleos stejskalleos

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nofaralfasi @stejskalleos @dosas @ekohl @adamruzicka @Gauravtalreja1